Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use connected faces from generated convex hull #80

Merged
merged 1 commit into from
Oct 25, 2022
Merged

Use connected faces from generated convex hull #80

merged 1 commit into from
Oct 25, 2022

Conversation

isaac-mason
Copy link
Contributor

@isaac-mason isaac-mason commented Aug 15, 2022 •
edited
Loading

donmccurdy
donmccurdy reviewed Aug 17, 2022
View reviewed changes
Copy link
Owner

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Gathering connected faces from the hull sounds good to me, but had a few questions on how we gather the vertex positions here.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@isaac-mason isaac-mason force-pushed the convex-hull-faces branch 3 times, most recently from b46dec9 to 063198a Compare August 18, 2022 11:52
@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2022 •
edited
Loading

Codecov Report

Merging #80 (3a13f24) into main (144fd6b) will increase coverage by 1.80%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
+ Coverage   77.94%   79.74%   +1.80%     
==========================================
  Files           3        3              
  Lines        1904     1940      +36     
  Branches       46       45       -1     
==========================================
+ Hits         1484     1547      +63     
+ Misses        420      393      -27
Impacted Files Coverage Δ
lib/ConvexHull.js 86.04% <100.00%> (+2.55%) ⬆️
src/index.ts 76.34% <100.00%> (-0.66%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@isaac-mason
Copy link
Contributor Author

isaac-mason commented Aug 18, 2022 •
edited
Loading

Updated to address comments. This approach reverts back to using setFromObject and now removes vertices not included in the convex hull (interior vertices).

I'll give this another pass shortly - and any feedback would be appreciated 🙂

@isaac-mason isaac-mason force-pushed the convex-hull-faces branch 2 times, most recently from 03c543f to 0d97b62 Compare August 18, 2022 14:00
@isaac-mason isaac-mason requested a review from donmccurdy August 18, 2022 23:43
donmccurdy
donmccurdy reviewed Sep 2, 2022
View reviewed changes
Copy link
Owner

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the slow reply here! Just had some questions on collectFacesAndVertices.

lib/ConvexHull.js Outdated Show resolved Hide resolved
lib/ConvexHull.js Show resolved Hide resolved
@isaac-mason
Use connected faces from generated convex hull
3a13f24 
Motivation:
- Generated convex hulls did not have connected faces, which is required for convex-convex collision in cannon-es
- Using the connected faces generated by ConvexHull addresses this
- Fixes donmccurdy#76
@isaac-mason
Copy link
Contributor Author

Sorry, was busy for a sec! 🙂

Marking as ready for review now. I've done some testing and this PR looks good to go.

As an aside, the generated shapes are now returning less vertices. Take this snippet as an example:

const cylinderGeometry = new CylinderGeometry(0.8, 1, 1, 32, 32);
const cylinderMesh = new Mesh(cylinderGeometry, meshBasicMaterial);
const cylinderMeshResult = threeToCannon(cylinderMesh, {
    type: ShapeType.HULL,
});

With the existing implementation, the generated hull had 1170 vertices. With the new implementation, the generated hull had 213 vertices.

Also, something appears to be indeterministic - the number of faces and vertices vary in both the existing and new implementations on each run. I'm not going to address that in this PR though.

@isaac-mason isaac-mason marked this pull request as ready for review October 11, 2022 14:22
@isaac-mason isaac-mason mentioned this pull request Oct 11, 2022
Draft
@donmccurdy
Copy link
Owner

donmccurdy commented Oct 14, 2022 •
edited
Loading

Thank you! Will do another review pass soon.

Also, something appears to be indeterministic - the number of faces and vertices vary in both the existing and new implementations on each run

The vertex positions displaced by small random amounts, here:

three-to-cannon/src/index.ts

Lines 246 to 255 in f570a1c

// Perturb.
const eps = 1e-4;
for (let i = 0; i < geometry.attributes.position.count; i++) {
geometry.attributes.position.setXYZ(
i,
geometry.attributes.position.getX(i) + (Math.random() - 0.5) * eps,
geometry.attributes.position.getY(i) + (Math.random() - 0.5) * eps,
geometry.attributes.position.getZ(i) + (Math.random() - 0.5) * eps,
);
}

This may be the cause. If I remember correctly, it's a simple way to avoid edge cases we would otherwise need to deal with in the convex hull algorithm, related to co-planar points. Not a problem, for our purposes!

@isaac-mason
Copy link
Contributor Author

Ah that looks like it, thanks. And yep, not really a big issue when cannon itself isn't deterministic!

@donmccurdy donmccurdy merged commit d1ee097 into donmccurdy:main Oct 25, 2022
@donmccurdy
Copy link
Owner

Thanks so much @isaac-mason! I've merged the changes and published v4.3.0.

@isaac-mason
Copy link
Contributor Author

Thanks for the help @donmccurdy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@donmccurdy donmccurdy donmccurdy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Physics simulation stops when generated convex hulls collide
3 participants
@isaac-mason @codecov-commenter @donmccurdy